-
Notifications
You must be signed in to change notification settings - Fork 0
I24 permission protection + metadata on course-evaluation page #113
Conversation
This does not fully work yet for review service
One of the things I realized while I was doing this
is that if I bind details of coordinators/reviewers in the course-evaluation state, we lose the ability for instant interface change effect. We have a couple of options to address this problem:
What do you think @MouseAndKeyboard ? |
Note: this needs to be thoroughly tested. Mistakes in this pull request will prevent any reviewer or coordinator from doing what they are supposed to do. Ideally, no existing features will be affected. You have to check this in the frontend by using an account that only has "just enough" permission to do the task. Eg. if you are doing a review, you should be able to do it with just the reviewer permission... and so on for other roles except administrator |
I have raised #115 to address changing interface state when updating user permission using "event-based" sockets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
const response = await services['users'].patch(userId, { | ||
perms: newPerms, | ||
services['users'].patch(userId, { | ||
$pull: { perms: {course_id:evaluationId,role:'Reviewer'}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and working
patch: [authenticate('jwt')], | ||
remove: [authenticate('jwt')], | ||
update: [authenticate('jwt'), roleBasedRestrictions(['Administrator'])], // Only Admin | ||
patch: [authenticate('jwt'), roleBasedRestrictions(['Coordinator'])], // Only Coordinator + Admin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be:
patch: [authenticate('jwt'), roleBasedRestrictions(['Coordinator', 'Administrator'])],
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nvm, I can see that the in role-based-restrictions.js it short-circuits for anyone with administrator
Currently just finalising testing, will merge when complete |
automated regression testing would have been great for this kind of things. However, I just got busy from setting it up. There are a couple of hurdles such as:
|
for future reference, use squash merge for feature branches to develop @MouseAndKeyboard |
Closes #24, Closes #71
Includes: